Intl: Add a new IntlListFormatter class #18519
Conversation
I wouldn't need an RFC if this is a straight forward addition to the existing Intl API which just maps to ICU in a straight forward fashion. I would like to see some smaller improvements, though:
The prefix makes sense to me. Using the |
|
Hi @TimWolla! Thanks for the suggestions - great ideas! I've pushed a commit that makes the class final and enabled those flags. |
| /** @not-serializable */ | ||
| /** @strict-properties */ |
There was a problem hiding this comment.
| /** @not-serializable */ | |
| /** @strict-properties */ | |
| /** | |
| * @not-serializable | |
| * @strict-properties | |
| */ |
To make it more clear that these two belong together.
| /** @strict-properties */ | ||
| final class IntlListFormatter { | ||
|
|
||
| /** @cvalue ULISTFMT_TYPE_OR */ |
There was a problem hiding this comment.
| /** @cvalue ULISTFMT_TYPE_OR */ | |
| /** @cvalue ULISTFMT_TYPE_OR */ |
| /** @cvalue ULISTFMT_TYPE_OR */ | ||
| public const int TYPE_OR = UNKNOWN; | ||
|
|
||
| /** @cvalue ULISTFMT_TYPE_UNITS */ |
There was a problem hiding this comment.
| /** @cvalue ULISTFMT_TYPE_UNITS */ | |
| /** @cvalue ULISTFMT_TYPE_UNITS */ |
| /** | ||
| * @param string $locale | ||
| * @param int $type | ||
| * @param int $width | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * @param string $locale | |
| * @param int $type | |
| * @param int $width | |
| */ |
| /** | ||
| * @return int | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * @return int | |
| */ |
| /** | ||
| * @return string | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * @return string | |
| */ |
| /** | ||
| * @param array $strings | ||
| * | ||
| * @return string|false | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * @param array $strings | |
| * | |
| * @return string|false | |
| */ |
| * @return string | ||
| */ | ||
| public function getErrorMessage(): string {} | ||
| } |
Thanks for the quick response! I'll make the changes tomorrow. :) |
| } | ||
|
|
||
| UErrorCode status = U_ZERO_ERROR; | ||
| if (U_ICU_VERSION_MAJOR_NUM >= 67) { |
There was a problem hiding this comment.
#if U_ICU_VERSION_MAJOR_NUM >= 67
| UErrorCode status = U_ZERO_ERROR; | ||
| if (U_ICU_VERSION_MAJOR_NUM >= 67) { | ||
|
|
||
| LISTFORMATTER_OBJECT(obj) = ulistfmt_openForType(ZSTR_VAL(locale), type, width, &status); |
There was a problem hiding this comment.
it is understood recent releases are more flexible however you do need to check type ... width does not contain silly values and if so throwing a ValueException (zend_argument_value_error with proper argument offset) then adding tests with silly values :)
|
windows CI failure is related otherwise |
| zend_long type = ULISTFMT_TYPE_AND; | ||
| zend_long width = ULISTFMT_WIDTH_WIDE; | ||
| ZEND_PARSE_PARAMETERS_START(1, 3) | ||
| Z_PARAM_STR(locale) |
There was a problem hiding this comment.
need to double check locale, e.g. ZSTR_LEN(locale) needs to be <= INTL_MAX_LOCALE_LEN
There was a problem hiding this comment.
to expand on that if its len is 0 then locale can fetch a default from intl_locale_get_default(). Might be simpler if locale is just char * then, just an advice.
This PR adds support for ICU's ListFormatter implementation by implementing a new IntlListFormatter class in PHP.
The class supports ANDs, ORs and UNIT format types. However, they are only available if ICU version is 67 or above. On older versions of ICU, we only allow TYPE_AND and WIDTH_WIDE - we need this because, from what I've understood, the minimum version ICU supported is 57, which was bumped from 50 last year.
The class API is pretty simple:
will display
I have some questions here: